Support scrolling with animation #4698
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luin
force-pushed
the
zh-animation-scrolling
branch
from
39b0d30 to
ed26d30
Compare
luin
force-pushed
the
zh-animation-scrolling
branch
from
b10e57d to
6af4ed5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements smooth scrolling animation support for Quill's scrollRectIntoView() and scrollSelectionIntoView() methods, similar to the native Element.scrollIntoView() API. The changes also include upgrades to browser-related test suite tools.
- Added optional
smoothparameter to scrolling methods for animated scrolling behavior - Upgraded Playwright and Vitest dependencies to address compatibility issues
- Enhanced test stability with timing adjustments and improved assertions
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/quill/src/core/utils/scrollRectIntoView.ts | Implements smooth scrolling by recording scroll operations and replaying with animation |
| packages/quill/src/core/quill.ts | Adds optional ScrollRectIntoViewOptions parameter to public scrolling methods |
| packages/quill/test/unit/core/quill.spec.ts | Adds test for smooth scrolling functionality and relaxes viewport ratio assertions |
| packages/quill/test/types/quill.test-d.ts | Updates TypeScript type definitions to include smooth scrolling options |
| packages/quill/package.json | Upgrades @playwright/test, @vitest/browser, and vitest dependencies |
| packages/quill/test/unit/vitest.config.ts | Removes deprecated slowHijackESM configuration option |
| packages/quill/test/fuzz/vitest.config.ts | Adds timeout and pool configuration for fuzz tests |
| packages/quill/test/e2e/utils/index.ts | Adds sleep utility function for test timing |
| packages/quill/test/e2e/list.spec.ts | Improves test stability with sleep delays and updated interaction methods |
| packages/quill/test/e2e/fixtures/index.ts | Shortens test fixture text content |
Comments suppressed due to low confidence (1)
packages/quill/test/unit/core/quill.spec.ts:1320
- The relaxed assertion from
toBe(1)totoBeGreaterThan(0.9)may mask actual issues with element positioning. Consider using a more precise threshold or adding a comment explaining why exact positioning cannot be expected.
).toBeGreaterThan(0.9);
Comment on lines
+157
to
+161
| // Revert all the changes in the scroll position | ||
| // and then scroll to the target position with smooth animation | ||
| records.forEach(({ element, top, left }) => { | ||
| element.scrollBy({ top: -top, left: -left, behavior: 'instant' }); | ||
| element.scrollBy({ top, left, behavior: 'smooth' }); |
There was a problem hiding this comment.
The smooth scrolling implementation reverts all scroll changes and then re-applies them with animation. This could cause visible flashing or jarring movement, especially with multiple nested scrollable elements. Consider implementing smooth scrolling without the revert-and-replay pattern, or add a check to skip this when no actual scrolling occurred.
Suggested change
| // Revert all the changes in the scroll position | |
| // and then scroll to the target position with smooth animation | |
| records.forEach(({ element, top, left }) => { | |
| element.scrollBy({ top: -top, left: -left, behavior: 'instant' }); | |
| element.scrollBy({ top, left, behavior: 'smooth' }); | |
| // Smoothly scroll to the target position if scrolling is required | |
| records.forEach(({ element, top, left }) => { | |
| if (top !== 0 || left !== 0) { | |
| element.scrollBy({ top, left, behavior: 'smooth' }); | |
| } |
Comment on lines
+23
to
+30
| await sleep(500); // internal(uiNode): wait for selectionchange to fire | ||
|
|
||
| expect(await editorPage.getSelection()).toEqual({ | ||
| index: 0, | ||
| length: 0, | ||
| }); | ||
|
|
||
| await page.keyboard.type('start '); | ||
| await sleep(500); // internal(uiNode): wait for selectionchange to fire |
There was a problem hiding this comment.
Hard-coded 500ms sleep delays make tests brittle and slow. Consider using more robust waiting mechanisms like waitFor with condition checks or event listeners instead of arbitrary timeouts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR added smooth behavior to
Quill#scrollRectIntoView(), similar to the nativeElement#scrollIntoView().Also upgraded browser-related test suite tools to address #33051